-
Notifications
You must be signed in to change notification settings - Fork 4
Feature/rdkemw 8587 dlsym #186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: test
Are you sure you want to change the base?
Conversation
* RDKEMW-4848: pass the proper handle to dsEnableHDCP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the dynamic library loading mechanism for device settings configuration to use a shared library handle instead of opening/closing the library multiple times. The changes introduce a centralized getDLInstance() and releaseDLInstance() pattern, and update all config loading methods to accept a handle parameter.
Key Changes
- Centralized dynamic library handle management with thread-safe singleton pattern
- Updated all config load methods to accept a
void* pDLHandleparameter - Added support for FrontPanelConfig in the initialization flow
- Removed the
parse_opt_flag()function and associated delay logic
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| ds/manager.cpp | Added getDLInstance/releaseDLInstance functions, refactored searchConfigs to use provided handle, updated Initialize/load methods to pass handle to configs, removed parse_opt_flag function and delay logic |
| ds/include/manager.hpp | Updated searchConfigs declaration to include pDLHandle parameter, removed parse_opt_flag declaration |
| ds/videoOutputPortConfig.hpp | Updated load method signature to accept pDLHandle parameter |
| ds/videoOutputPortConfig.cpp | Refactored load to accept handle parameter, moved symbol search variables into conditional block, added dynamic vs old config loading logic |
| ds/videoDeviceConfig.hpp | Updated load method signature to accept pDLHandle parameter |
| ds/videoDeviceConfig.cpp | Refactored load to accept handle parameter, added dynamic vs old config loading logic |
| ds/audioOutputPortConfig.hpp | Updated load method signature to accept pDLHandle parameter |
| ds/audioOutputPortConfig.cpp | Refactored load to accept handle parameter, added dynamic vs old config loading logic |
| ds/include/frontPanelConfig.hpp | Moved load method from private to public, added m_isFPConfigLoaded member, updated load signature to accept pDLHandle |
| ds/frontPanelConfig.cpp | Removed load call from getInstance, added load validation checks with m_isFPConfigLoaded, refactored load to accept handle parameter and support dynamic/old config loading |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ds/frontPanelConfig.cpp
Outdated
| configuration.pKIndicators[i].colorMode)); | ||
| } | ||
| } | ||
| if (pDLHandle) |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the FrontPanelConfig::load method, the check for pDLHandle uses a different null comparison pattern (pDLHandle without nullptr comparison) compared to other config files which consistently use "nullptr != pDLHandle". Consider using "nullptr != pDLHandle" for consistency with the rest of the codebase.
| if (pDLHandle) | |
| if (nullptr != pDLHandle) |
| } | ||
| m_isFPConfigLoaded = true; | ||
| INT_INFO("Exit function\n"); | ||
| return; |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The explicit return statement at the end of a void function is unnecessary. Since this function returns void, the return statement at line 554 can be removed as the function will automatically return at its end.
| return; |
ds/videoDeviceConfig.cpp
Outdated
|
|
||
| if ( false == isDynamicConfigLoad) | ||
| { | ||
| INT_INFO("Read Old Configs\n"); |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The log message format for old config loading is inconsistent. In videoDeviceConfig.cpp line 164 it says "Read Old Configs" while other files (videoOutputPortConfig.cpp line 435, audioOutputPortConfig.cpp line 235, frontPanelConfig.cpp line 482) consistently use "Using OLD config loading". Consider updating this message to match the consistent pattern used elsewhere.
| INT_INFO("Read Old Configs\n"); | |
| INT_INFO("Using OLD config loading\n"); |
ds/manager.cpp
Outdated
| void* getDLInstance() | ||
| { | ||
| std::lock_guard<std::mutex> lock(gDLMutex); | ||
| dlerror(); // clear old error | ||
| if (nullptr == gDLHandle){ | ||
| gDLHandle = dlopen(RDK_DSHAL_NAME, RTLD_LAZY); | ||
| } | ||
| INT_INFO("%d:%s: DL Instance '%s'\n", __LINE__, __func__, (nullptr == gDLHandle ? "NULL" : "Valid")); | ||
| return gDLHandle; | ||
| } |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getDLInstance function does not log or handle the error when dlopen fails. The old implementation would log the error message from dlerror(), but this new implementation only logs whether the handle is NULL or Valid. Consider logging the actual error message from dlerror() when dlopen fails to aid in debugging issues with loading the dynamic library.
ds/manager.cpp
Outdated
| void* pDLHandle = getDLInstance(); | ||
| device::AudioOutputPortConfig::getInstance().load(pDLHandle); | ||
| device::VideoOutputPortConfig::getInstance().load(pDLHandle); | ||
| device::VideoDeviceConfig::getInstance().load(pDLHandle); |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Manager::load() method calls load for AudioOutputPortConfig, VideoOutputPortConfig, and VideoDeviceConfig, but does not call FrontPanelConfig::getInstance().load(pDLHandle). This is inconsistent with Manager::Initialize() which calls load on all four config classes including FrontPanelConfig. This inconsistency could lead to FrontPanelConfig not being properly loaded when Manager::load() is called.
| device::VideoDeviceConfig::getInstance().load(pDLHandle); | |
| device::VideoDeviceConfig::getInstance().load(pDLHandle); | |
| device::FrontPanelConfig::getInstance().load(pDLHandle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>
Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>
Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>
Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
…logging format Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
Reason for change: Address review comments. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: Addressed review comments. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
ef2294f to
b8cdf99
Compare
No description provided.